-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add Result field to ResolverContext #301
Conversation
Resolver context has a reference to the parent context as of #294.
Perhaps this would be better represented by |
this PR collide with #302 🤔 This PR introduce |
codegen/object.go
Outdated
rctx.PushIndex({{.index}}) | ||
defer rctx.Pop() | ||
{{.arr}} = append({{.arr}}, func(ctx context.Context) graphql.Marshaler { | ||
rctx := &graphql.ResolverContext{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is strange. array marshalling isn't a resolver, and it shouldn't need a new context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is right.
this change required default:
case under case len(remainingMods) > 0 && remainingMods[0] == modList:
.
I'll move this code to right place and add some arguments to method signature.
🤔 ❓
codegen/templates/object.gotpl
Outdated
@@ -30,6 +30,8 @@ func (ec *executionContext) _{{$object.GQLType}}(ctx context.Context, sel ast.Se | |||
ctx = graphql.WithResolverContext(ctx, &graphql.ResolverContext{ | |||
Object: {{$object.GQLType|quote}}, | |||
}) | |||
{{else}} | |||
graphql.GetResolverContext(ctx).Result = obj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably happen using res around field.gotpl:62, here its going to be repeated for every child
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah... make sense...
It was a bit different from what I thought.
so I'll write what I want to do.
@vektah Result and ParentObject is not equals. |
I noticed that there are multiple ideas of ResolverContext...
I think the 2 is more convenient. Also, when users browse / create ResolverContext, it is not known where asynchronous processing will be occurred. |
It might be well refactored. 🤔 |
I still think its a bit weird to be creating a new resolver context for each result, currently all of that state stays on the stack as method args for normal resolvers. Perhaps this should work the same? see #314 for an alternative. |
// indicies tracks the array indicies only. all field aliases come from the context stack | ||
indicies []int | ||
// The index of array in path. | ||
Index *int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then again I think this is a bit nicer.
add Result field to ResolverContext
refs #289
works fine vvakame/til@8ddb178#diff-533b5224b1fd11ab427fe7114c0fdc13